feat(sdk-core): add sync HTTP-client warm-up to SdkWarmUp.prime() for CRaC priming#7080
Conversation
e4dfbde to
5b172b4
Compare
5b172b4 to
82c723b
Compare
bbf82e3 to
44b6b00
Compare
| public class Apache5HttpClientWarmUpTest { | ||
|
|
||
| @Rule | ||
| public WireMockRule mockServer = new WireMockRule(0); |
There was a problem hiding this comment.
We have a test for each HTTP client instead of a shared suite in test/http-client-tests. A shared suite there would create a cyclic dependency, because sdk-core has test dependencies on apache-client and netty-nio-client (sdk-core -> apache-client -> http-client-tests -> sdk-core). I filed JAVA-9020 to refactor this on master; once fixed, these tests can move to test/http-client-tests as a single SyncHttpClientWarmUpTestSuite shared across all HTTP clients.
There was a problem hiding this comment.
A separate centralized package would be great, but maybe we can have a dedicated test package for testing CRaC components instead of using http-client-tests
| <version>${awsjavasdk.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
44b6b00 to
2f5b864
Compare
| } | ||
|
|
||
| // AWS_DEFAULT_REGION is an environment-variable-only fallback with no system-property equivalent. | ||
| private static final class AwsDefaultRegionEnvVar implements SystemSetting { |
There was a problem hiding this comment.
AFAIK, we don't use AWS_DEFAULT_REGION for determining endpoints in the SDK (only DefaultsMode). Should we leave this out?
| */ | ||
| public URI stsEndpoint() { | ||
| // URL-encode the region before putting it in the host, same as Region.of(String). | ||
| return URI.create("https://sts." + SdkHttpUtils.urlEncode(resolveRegion()) + ".amazonaws.com/"); |
There was a problem hiding this comment.
might benefit from using
There was a problem hiding this comment.
alternatively, build using the URI constructor will also validate it for correctness
There was a problem hiding this comment.
Good call, adopted HostnameValidator. The region is now validated with validateHostnameCompliant; if it isn't a valid hostname component, we log and fall back to us-east-1 rather than propagate the exception, since endpoint resolution runs inside the best-effort warm-up and must not break priming.
| private final String body; | ||
| private final String contentType; |
There was a problem hiding this comment.
The ctor is private; are these necessary?
There was a problem hiding this comment.
Good catch, they weren't necessary. The class started as a small spec (method/body/content-type) intended to be shared by the sync and async warmers, but once the warm-up settled on a simple GET those fields were always null. I've removed WarmUpRequest entirely.
| * Resolves the regional STS endpoint ({@code https://sts.<region>.amazonaws.com/}) used by the CRaC HTTP-client warm-up. | ||
| * |
There was a problem hiding this comment.
Is this class intended to resolve other service endpoints in the future?
There was a problem hiding this comment.
No. It is specific to whichever single service we pick for the warm-up (currently STS). It always returns one endpoint for that hardcoded service. If we later switch the warm-up to another service (e.g. DynamoDB), this class would build that endpoint instead. Renamed stsEndpoint() to endpoint() to reflect that.
| * matters for JIT priming. | ||
| */ | ||
| @SdkInternalApi | ||
| public final class WarmUpRequest { |
There was a problem hiding this comment.
Not really sure about the usefulness of this class. Does it make more sense to just have a utility class/factory just creates the HttpExecuteRequest directly?
| public class Apache5HttpClientWarmUpTest { | ||
|
|
||
| @Rule | ||
| public WireMockRule mockServer = new WireMockRule(0); |
There was a problem hiding this comment.
A separate centralized package would be great, but maybe we can have a dedicated test package for testing CRaC components instead of using http-client-tests
7421ffc to
b4e9f64
Compare
b4e9f64 to
072736d
Compare
70fb508
into
feature/master/crac_auto_priming_support
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
Motivation and Context
This change adds the second half of the hybrid priming approach: one real GET per sync HTTP client on the classpath, so that network path is JIT-compiled into the CRaC snapshot. It also adds the region/endpoint resolution shared with the future async warm-up.
Modifications
SdkWarmUp.prime()now runs the per-service providers and then the HTTP-client warm-up, both inside the existing idempotency guard. The HTTP-client warm-up is best-effort: a network failure during init is logged and swallowed so the rest of priming completes.New internal types in
sdk-core:RegionEndpointResolver(internal.crac): resolves the region from theaws.regionsystem property orAWS_REGIONenvironment variable, falling back tous-east-1, and buildshttps://sts.<region>.amazonaws.com/. It reads only system properties and environment variables, avoiding the SDK region-resolution chain (IMDS, profile file) during priming. The region is validated withHostnameValidator; a value that is not a valid hostname component is rejected and the default region is used, so a malformed region cannot alter the endpoint host.AWS_DEFAULT_REGIONis not consulted, matching how the SDK resolves a region from system settings for endpoints.SyncHttpClientWarmer(internal.http.loader): discovers every syncSdkHttpServicethrough the existing package-privateSdkServiceLoader, builds each client, sends the warm-up GET, drains the response body, and closes the client.WarmUpDiscovery: the discover-and-iterate loop shared by the per-service and HTTP-client warm-up. It skips an element that fails to load or run, includingLinkageError(a client jar present without its transitive dependencies), so one broken element does not stop the others.HttpClientWarmerinterface plusHttpWarmupInvoker/ClasspathHttpWarmupInvoker:prime()calls one invoker that owns the set of warmers. The async warmer is added to that set later without changingSdkWarmUporSyncHttpClientWarmer.Tests for the warm-up against every sync HTTP client live in a new
test/warmup-testsmodule rather than in each client module, avoiding thesdk-core -> apache-client -> http-client-tests -> sdk-corecyclic dependency.Testing
Added Junits test
Screenshots (if appropriate)
Types of changes
License